Review: Correct generated CLI enum values (#3572)

Verified the core bug is real and the fix is correct. CommandArgumentBuilder.ParseEnum (src/ModularPipelines/Helpers/Internal/CommandArgumentBuilder.cs:344-354) only reflects for EnumValueAttribute — it never looked at [Description]. So every generated enum still decorated with [Description("azure-native")] etc. was silently falling back to rawValue.ToString() (e.g. "AzureNative", "Rawjson", "__82540em") when building CLI args. That's a genuine, previously-silent correctness bug affecting real Docker/Minikube/Pulumi command invocations, not a cosmetic rename. Confirmed the source generator (Generators/EnumGenerator.cs:109) already emits [EnumValue(...)] for new output, so this PR is just bringing stale checked-in generated files back in sync with current generator behavior, plus adding a gate so they can't drift again. Good catch, good fix.

CI gate (Test-GeneratedEnumAttributes.ps1) looks solid — git grep -l --fixed-strings '[Description(' -- 'src/**/*.Generated.cs' correctly distinguishes "no matches" (exit 1) from a real grep failure (exit >1), and I confirmed 0 remaining [Description( hits in generated files in the current tree.

Duration-suffix fix is well targeted and tested — IsDurationUnit + the new CliDocumentationScraperBaseTests cover the "ms|s|m|h" case that produced the bogus DockerContainerCreateHealthInterval-style enums, and confirm real enums like plain|json|tty still detect correctly.

One gap worth closing: the Pulumi "repeatable/comma-separated" fix has no regression guard

The three deleted Pulumi enums (PulumiDeploymentSettingsEditOidcAwsPolicyArn, ...PathFilter, ...RemoveEnv) had values Repeatable/CommaSeparated — clearly the CLI help's parenthetical annotation "(repeatable, comma-separated)" being misread as the option's accepted value set. Pulumi is scraped via PulumiCliScraper : CobraCliScraper, not CliDocumentationScraperBase — so this bug came from a different detection path (CobraHelpTypeDetector / HeuristicTypeDetector / DescriptionEnumValueParser), which this PR doesn't touch at all. The only enum-quality fix and test added (IsDurationUnit) lives exclusively in CliDocumentationScraperBase, which Pulumi doesn't use.

Concretely: nothing stops a future re-scrape of Pulumi (or any other CobraCliScraper-based tool) from reproducing the exact same "annotation phrase treated as enum values" bug that was just manually fixed by deleting these three files. Right now the fix is "delete the bad generated output," not "prevent the generator from producing it again," which is inconsistent with how the Docker duration-unit case was handled (root-caused in the scraper + regression test).

Suggest either:

• Adding an equivalent denylist/heuristic in DescriptionEnumValueParser.TryParseValues (or wherever the Cobra-path enum candidates get validated) that rejects annotation-only words like repeatable/comma-separated/optional the same way IsDurationUnit rejects unit suffixes, with a test mirroring CliDocumentationScraperBaseTests, or
• If the actual originating detector is elsewhere and I've mis-traced it, at minimum a comment/test pinning down where these three enums actually came from, so the fix and the regression test live next to the real root cause rather than only covering the Docker-specific path.

Everything else — the IEnumerable<string> conversion for the repeatable Pulumi options, the string? conversion for Docker's free-form duration options, the workflow addition of the new gate step — looks correct and appropriately scoped. No other issues found.

Re-review (head 47cfdff9)

Checked this against my previous review comment, which flagged one gap: the Pulumi "repeatable, comma-separated" pseudo-enum bug came from the Cobra scraping path (DescriptionEnumValueParser / HeuristicTypeDetector), not the CliDocumentationScraperBase.IsDurationUnit path that got the only regression test in the original fix.

That gap is resolved. Commit bda47b1e added:

• DescriptionEnumValueParserTests.TryParse_Rejects_Repeatability_Annotations — traced through TryParse: none of the three sample descriptions match ExplicitValuesPattern, QuotedOneOfValuesPattern, or ContextualParenthesizedValuesPattern, so it correctly returns null today.
• HeuristicTypeDetectorTests.DetectType_Treats_Repeatability_Annotations_As_StringList — traced through AnalyzeOption: "repeatable" sets acceptsMultipleValues = true, the enum lookup returns null, and it falls through to CliOptionType.StringList with no enum values.

This is real regression coverage against the code path that produced the original bug, not just deleting the bad output and hoping — consistent with how the Docker duration-unit case was handled.

Since then, 45a8392f fixed the one remaining loose end: DockerBuilderCompatibilityTests.BuilderEnumOptionRendersCanonicalSwitch expected --progress=Plain (the old broken ToString() fallback value) and now correctly expects --progress=plain (the canonical [EnumValue("plain")] value). Confirmed this matches DockerBuilderHistoryLogsProgress.Generated.cs and is consistent with the rest of the file, which already asserted lowercase values elsewhere. The merge commit on top (5512111f) only brings in unrelated main changes to HTTP test infra — nothing from this PR's own diff.

No new issues found. All three review rounds' concerns are addressed:

• Stale [Description] → [EnumValue] migration: correct, verified ParseEnum only reflects on EnumValueAttribute.
• Docker duration-suffix and Pulumi repeatability pseudo-enums: root-caused and tested at the actual detection code paths, not just patched at the generated-file level.
• Test-GeneratedEnumAttributes.ps1 CI gate: confirmed zero remaining [Description( hits in generated files.

One optional, non-blocking thought for the future: the new CI gate is a git grep --fixed-strings '[Description(' textual check. It works today because the generator only ever emits the short attribute form, but it's a heuristic that could silently stop catching regressions if someone ever changes that emission style (e.g. a fully-qualified [System.ComponentModel.Description(...)]). The repo already has a stronger pattern for this class of problem — Test-StageGeneratedChanges.ps1 regenerates and diffs against the checked-in output. If this gate ever needs hardening, folding the enum-attribute check into that regenerate-and-diff flow (rather than a second, independent grep heuristic) would make it structurally impossible to drift, instead of relying on the grep pattern staying in sync with the generator's output format. Not asking for a change now — just flagging it as the more robust shape if this gate ever needs to evolve.

Approving.

Re-review (head b47bddd0)

Diffed the current PR head against the previously-approved commit (5512111f, approved 2026-07-30T19:33:39Z) — the only change since is b47bddd0, a merge of main (adds an unrelated PipelineCommand/module-selection feature and a dependency bump). It touches none of this PR's own files; the enum/generator diff is byte-identical to what was already reviewed.

Recapping the three rounds already resolved on this PR:

• Root cause confirmed correct: CommandArgumentBuilder.ParseEnum only reflects [EnumValueAttribute], so generated enums still carrying [Description] silently fell back to rawValue.ToString() at the CLI boundary (e.g. AzureNative instead of azure-native). Migrating stale generated files to [EnumValue(...)] fixes real, previously-silent command-arg bugs, not just a rename.
• Docker duration-suffix pseudo-enums: correctly root-caused in CliDocumentationScraperBase.IsDurationUnit, with regression tests (CliDocumentationScraperBaseTests) pinning the fix at the actual detection path.
• Pulumi repeatability pseudo-enums (the gap flagged in my first review round): resolved in bda47b1e with regression tests at both the DescriptionEnumValueParser.TryParse and HeuristicTypeDetector.AnalyzeOption boundaries — traced through and confirmed these assert real behavior (annotation phrases don't match any of the enum-detection regexes, and acceptsMultipleValues correctly routes to StringList), not just documentation of intent.
• CI gate (Test-GeneratedEnumAttributes.ps1): confirmed zero remaining [Description( hits in generated files; the exit-code handling correctly distinguishes "no matches" from a real grep failure.
• Docker compatibility test fix (45a8392f): updated a stale assertion (--progress=Plain → --progress=plain) to match the now-canonical [EnumValue] output — verified against DockerBuilderHistoryLogsProgress.Generated.cs.

No new issues. This PR remains approved — nothing outstanding.

Code review — follow-up on latest commit

Re-reviewed the state of the PR since my last pass (bda47b1e). The only substantive change since then is 45a8392f9 ("test(docker): expect canonical enum value"), which fixes the Ubuntu CI failure by updating DockerBuilderCompatibilityTests to expect --progress=plain instead of --progress=Plain.

Verified correct: traced this through CommandArgumentBuilder.ParseEnum (src/ModularPipelines/Helpers/Internal/CommandArgumentBuilder.cs:343) — it resolves enum arguments via the field's EnumValueAttribute, and the current generated DockerBuilderHistoryLogsProgress.Generated.cs declares [EnumValue("plain")] Plain. So at runtime the argument really does serialize to plain, not Plain. The test was asserting stale, pre-fix behavior (accidentally matching the C# enum member name rather than the CLI's actual accepted value) — updating the expectation to the canonical lowercase value is the right fix, not a workaround.

Everything else in the diff (generated enum migration from DescriptionAttribute to EnumValueAttribute, the Docker/Pulumi pseudo-enum → string/IEnumerable<string> conversions, the DescriptionEnumValueParser/HeuristicTypeDetector regression tests for the repeatability-annotation bug, and the new Test-GeneratedEnumAttributes.ps1 CI gate) was already covered and confirmed correct in my earlier review comment — no new concerns there.

No issues found. This is a solid close-out of #3503.